fix(studio): scope timeline context targets - #2708

Verdict: APPROVE at 6921c585d1.

Context-menu-scoping done right: two-phase resolution (render-time + action-fire), session-epoch tagged at open, LIVE-elements re-resolution at fire, explicit animationId propagation, aggressive stale-target auto-dismiss. Consistent with the Family E lifecycle-scoping language. No blockers.

What I verified

Two-phase target resolution (TimelineOverlays.tsx:69-95, 148-149)

resolveTimelineContextElement at render time requires ALL THREE conditions to return non-null:

1. targetSessionEpoch === sessionEpoch — the project session captured when the menu opened must match the current one.
2. selectedElementId === identity — the captured target must still BE the current selection.
3. elements.find(...) !== undefined — the element must still exist in the rendered slice.

Any one failing → null → useEffect at :151-157 auto-dismisses the menu (setKfContextMenu(null) / setClipContextMenu(null)). Menu state is never left stranded with an unresolvable target.

readCurrentElement at action-fire time re-resolves against elementsRef.current (LIVE elements, NOT the closed-over render-time elements) via the same resolveTimelineContextElement predicate. Every action call is guarded:

• onDelete (:182-185): if (!readCurrentElement(...)) return;
• onDeleteAll (:186-189): const element = readCurrentElement(...); if (element) onDeleteAllKeyframes(element, animationId);
• onMoveToPlayhead (:190-197): same pattern
• onSplit (:208-211): same
• onDelete clip (:212-217): same, plus pinZoomBeforeEdit() before delete

Two layers of defense: render-time keeps the menu visible only while its target is valid, action-fire re-verifies against a possibly-newer state right before mutating.

Animation-scoped delete-all (useTimelineEditCallbacks.ts:189-210)

onDeleteAllKeyframes(element, animationId?) — new optional parameter. Semantics:

• With animationId: animations.filter(a => a.id === animationId) → only the clicked lane.
• Without: legacy layer-wide behavior (animation.keyframes truthy) → every keyframed tween.
• Stale animationId matches nothing → deletes nothing. Directly quoted in the code comment (:198-206): "A stale id matches nothing and deletes nothing, which is the point: it never falls back to a lane the user did not click." Perfect stale-target rejection.

The legacy "delete-all with no id" behavior also carries a comment-documented bug fix (from an earlier PR): .filter(animation.keyframes) catches EVERY keyframed tween, not just the first — the "Delete All Keyframes did half the job" case.

Session-stamped track-gap menu (useTrackGapMenu.ts:60-167)

Three-way session-epoch defense on the gap menu:

• openGapMenu(anchor) stamps sessionEpoch: usePlayerStore.getState().timelineSessionEpoch at open (:155-158).
• gapMenuLaneElements derives to null when anchor.sessionEpoch !== current (:65), which nulls the whole gapMenuModel downstream.
• closeTrackGap and closeAllTrackGaps re-check sessionEpoch === current at commit (lines 110, 133) — re-read from store, not just from the captured value.
• useEffect at :165-167 auto-dismisses menu on session-change: if (menu.sessionEpoch !== sessionEpoch) dismiss().

Additionally, gapMenuModel derives from LIVE tracks (prop) not a snapshot at open — so concurrent edits to the target lane reflect in the open menu (e.g., "Close gap" enabled/disabled updates as elements move). Correct choice for a menu that stays open across edits.

Foreign-target rejection converges to no-op on every mutation path:

• Keyframe menu action: readCurrentElement → null → early return, no store write.
• Clip menu action: same.
• onDeleteAllKeyframes with stale animationId: filter → empty array → early return at :207.
• Gap menu commit: session mismatch → early return before commitCloseTrackGap / commitCloseAllTrackGaps fires.

No mutation path can accept a stale target as-a-fallback to the current selection.

No callback closures over per-row refs (my #2705 class). Verified: resolveTimelineContextElement is a pure function; readCurrentElement reads elementsRef.current (top-level ref); useTrackGapMenu's hooks live in Timeline.tsx (stable ancestor). The useEffect closures over keyframeElement / clipElement are safe because those are derived from elements at render time — if elements updates, effect re-runs with the new derived value.

Non-blockers

1. resolveTimelineContextElement requires selectedElementId === identity in addition to the identity/session/existence checks (:78). If UX intent is ever "right-click a non-selected item without selecting it first," the menu would close immediately. Per convention, right-click typically selects on pointerdown, so the invariant holds at open time — but any future divergence in that pattern would need this line to relax.

2. useEffect at :151/155 IS a state-syncing effect (auto-dismisses stale menu). Pacific convention discourages useEffect for state syncing, but the alternative (leaving invalidated menu state stranded) is worse. Justified exception; if a lint rule flags this, an eslint-disable comment with rationale would document it.

3. Two menus (kf + clip) can conceptually be open simultaneously. Not enforced here — assumed mutually-exclusive by the caller (Timeline.tsx opens one at a time via right-click gesture routing). Non-blocker as long as the caller invariant holds.

4. onDeleteAll(_element, animationId) uses _element prefix for the ignored arg (:186). If future code accidentally reads _element instead of the re-resolved local, it would use render-time-stale data. Low risk given the underscore convention is well-established.

5. Selection-change auto-dismisses the menu via selectedElementId !== identity. If the user has a keyframe menu open and clicks a diamond in a DIFFERENT clip while the menu is open, the previous menu closes. Expected but worth noting.

Family E stack ack

Family E, 5 of 7. Base: main (post-#2704/#2705/#2706/#2707 merges). Next: #2709. 40 focused context-target tests + typecheck + lint + format green per PR body. CI: 19 passing, 12 running, 0 failing at time of review.

— Review by Via

Reviewed at 6921c585d1; PR already merged as 423c5ffa. Posting as a follow-up FYI in case any of it wants to roll forward into the remaining Family E work (#2710 and later cleanup).

Nothing here was a blocker. The scope-the-context-target machinery is solid — resolveTimelineContextElement, the session-epoch stamps on all three menu producers (useTimelineKeyframeHandlers.ts:553, Timeline.tsx:560, useTrackGapMenu.ts:157), and the click-time re-resolve wrappers in TimelineOverlays.tsx:175-217 compose cleanly. The stale-animationId no-op path in useTimelineEditCallbacks.ts:206-207 has both positive and negative test coverage. Good pattern to carry into remaining primitives.

Four items worth surfacing:

Concerns

1. Removing eager clearKeyframeCacheForElement opens a stale-diamond window on class/descendant-selector elements. useGsapKeyframeOps.ts:331-350 drops the eager cache clear and relies on the deferred commitMutation → onCacheInvalidate → useGsapCacheVersion.bump → usePopulateKeyframeCacheForFile → replaceKeyframeCacheForFile cascade. That works — for elements whose selection carries selection.id. When selection.id is null (any element resolved via class or descendant selector, e.g. .dot tweens rescued by DOM-id-less paths), updateKeyframeCacheFromParsed's targetIds = selectionId ? [selectionId] : [] branch at gsapKeyframeCacheHelpers.ts:117 skips the delete step, and only the file-wide replaceKeyframeCacheForFile on the AST refetch (an HTTP round-trip, 200-1000ms) eventually clears the entries. In that window the diamonds stay drawn on the deleted lane. The removed inline comment above the old clearKeyframeCacheForElement call explicitly documented "the commit path doesn't return parsed animations, so the keyframe cache is never refreshed" — that constraint still holds for remove-all-keyframes (it collapses to a static hold). Under the HF flicker lens this is the same class of regression Miao has been reporting.

2. TimelineOverlays.test.ts (new) tests only the pure resolveTimelineContextElement helper — the actual PR change lives in the component wrappers, and those have no component-level tests. The auto-dismiss useEffects at TimelineOverlays.tsx:151-153 and :155-157, the click-time readCurrentElement gates at :186, :190, :200, :205, and the state={{ ...kfContextMenu, element: keyframeElement }} reconstruction at :180 are the meat of the PR. A regression that removes any of them, or reverts to dispatching the stale captured keyframeElement/clipElement, passes the current test file. Consider one <TimelineOverlays /> render test that opens a menu, changes selection or removes the element, and asserts the menu auto-dismisses; and a positive-path click test that asserts the callback fires with the live-model element.

3. <KeyframeDiamondContextMenu state={{ ...kfContextMenu, element: keyframeElement }} onClose={…} onDelete={…} onDeleteAll={…} onMoveToPlayhead={…} /> at TimelineOverlays.tsx:180-207 creates a fresh state object and four fresh closures on every parent commit. TimelineOverlays subscribes to state.selectedElementId and state.timelineSessionEpoch (:127-128) and receives elements from Timeline, so it re-renders on every playhead tick / drag commit / store mutation. The memo(function KeyframeDiamondContextMenu(...)) wrap becomes cosmetic — memo never skips. The menu itself is cheap so no user-visible flicker today, but the memo boundary now advertises a guarantee the call site can't honor. Options: useMemo the spread on [kfContextMenu, keyframeElement] and useCallback the handlers, or drop the memo wrap.

Nits

4. TimelineEditCallbacks.onChangeKeyframeEase at timelineCallbacks.ts:77 has zero consumers. grep -rn "onChangeKeyframeEase" packages/studio/src returns exactly one hit (the declaration). Per HF pattern, drop until the producer + consumer land together — an optional-callback interface field with no wiring invites future callers to assume "supported."

Minor stack-carry-forward suggestions:

• KeyframeDiamondContextMenuState.sessionEpoch is optional (:11) but ClipContextMenuState.sessionEpoch is required (TimelineOverlays.tsx:20) — resolveTimelineContextElement's targetSessionEpoch !== sessionEpoch check silently auto-dismisses any unstamped producer with no type error. Consider tightening KeyframeDiamondContextMenuState.sessionEpoch to required (MotionPathOverlay's local-render usage aside).
• useGsapKeyframeOps.test.tsx:206's "lets the successful commit refresh own delete-all cache invalidation" — the mock commitMutationSafely is a bare unresolved Promise, so the test only proves the eager clear is gone, not that any refresh actually runs. A name like "defers cache invalidation to the commit cascade" reads honestly.
• useTrackGapMenu.test.tsx covers only the stale-epoch negative path; a positive-path assertion that closeTrackGap / closeAllTrackGaps DO route through commitCloseTrackGap / commitCloseAllTrackGaps on epoch match would guard the epoch-check inversion regression.
• useTrackGapMenu.test.tsx:9 uses bare (globalThis as { … }) where the three other new test files in this PR use (globalThis as unknown as { … }) (the CONTRIBUTING.md §Type-casts style).

— Review by Rames D Jusso